Create general PoolingOp and decompose to MaxPool2d #1154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LPanosTT
requested review from
sdjordjevicTT,
svuckovicTT,
mtopalovicTT,
rpavlovicTT,
nobradovictt,
jserbedzijaTT,
jnie-TT,
nsmithtt and
mrakitaTT
as code owners
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
2 times, most recently
from
ac10e4e to
bb05d0c
Compare
LPanosTT
requested review from
kmabeeTT,
AleksKnezevic and
pilkicTT
as code owners
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
3 times, most recently
from
5665196 to
21e4b35
Compare
nsmithtt
reviewed
Nov 4, 2024
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
3 times, most recently
from
aa08eb0 to
25fee5d
Compare
nsmithtt
approved these changes
Nov 5, 2024
lib/Conversion/TTIRToTTIRDecomposition/TTIRToTTIRDecomposition.cpp
Outdated
Show resolved
Hide resolved
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
3 times, most recently
from
f55ca0d to
68084e8
Compare
jnie-TT
approved these changes
Nov 7, 2024
svuckovicTT
requested changes
Nov 8, 2024
There was a problem hiding this comment.
Thanks for packing the reshapes to the conversion itself, that makes it a lot cleaner!
Requesting changes for the following reasons:
- replaceAllUsesWith needs to be removed
- in more than one file, I've seen a mix of camelCase and snake_case, can you please go over all the files and correct this?
- personally, I'd like to see more comments - pooling ops are among the most complicated to work with but with properly commented code, it gets easier :)
lib/Conversion/TTIRToTTIRDecomposition/TTIRToTTIRDecomposition.cpp
Outdated
Show resolved
Hide resolved
lib/Conversion/TTIRToTTIRDecomposition/TTIRToTTIRDecomposition.cpp
Outdated
Show resolved
Hide resolved
| ttir::ConvolutionOp op, | ||
| const std::vector<int64_t> ttnn_convolution_kernel_layout) { | ||
| static std::vector<TransposeDims> | ||
| generateTransposeIndicesLeftToRight(std::vector<int64_t> current_layout, |
There was a problem hiding this comment.
All these transpose methods are hard to follow. A comment (with an example) above each method would make it much easier to come back to.
Additionally, it's somewhat noisy to keep these method in this file, can they be moved to a util file of sorts?
There was a problem hiding this comment.
I've cleaned them up and removed a number of them. Also added comments.
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
3 times, most recently
from
41faf8a to
4b23448
Compare
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
2 times, most recently
from
f392818 to
b5011ed
Compare
svuckovicTT
approved these changes
Nov 12, 2024
| * result at the end of the sequence | ||
| */ | ||
| static Value | ||
| generateTransposeSequence(Value input, PatternRewriter &rewriter, |
There was a problem hiding this comment.
Consider renaming generateTransposeSequence to applyTransposeIndices.
There was a problem hiding this comment.
I think generateTransposeOps might be better so that the fact its creating ops in the IR is explicit
lib/Conversion/TTIRToTTIRDecomposition/TTIRToTTIRDecomposition.cpp
Outdated
Show resolved
Hide resolved
lib/Conversion/TTIRToTTIRDecomposition/TTIRToTTIRDecomposition.cpp
Outdated
Show resolved
Hide resolved
maxpool2d Use output memory config attribute in runtime, add silicon tests
LPanosTT
force-pushed
the
lpanos/general_pooling
branch
from
b5011ed to
d36e4f2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ttir.max_pool2d, the reshapes are now inserted upon conversion tottnn.max_pool2dThe PoolingOp is meant to closely resemble the
stablehlo.reduce_windowop. One of its features is that it can take any number of inputs and Pool them all simultaneously, and return a tuple with all the outputs. In the decomposition pass I convert this into multiple individual maxpool2d ops if need be. There is a test for this.